-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQS-412 | Active Orders Query: SSE #518
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, including the addition of a logger to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
90fb6d3
to
c3cd2c4
Compare
} | ||
|
||
if orders.Error != nil { | ||
a.Logger.Error("GET "+c.Request().URL.String(), zap.Error(orders.Error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to respond with an error here @crnbarr93?
It would look something like: "id: \ndata: { "message": "error" }\n\n"
c3cd2c4
to
eb20ef5
Compare
eb20ef5
to
2e5aabd
Compare
Nice! Sanity check: are there any tests that it would make sense to add to reach the 50% min coverage threshold so that we keep on improving on coverage generally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Just some nits and a test request.
I'm seeing Sonar code duplication check also failing - is there anything we can address to make it pass?
Also, what is our current integration plan with the FE?
// Fetch orders duration | ||
d := 5 * time.Second | ||
|
||
// Result channel | ||
c := make(chan orderbookdomain.ActiveOrder, 50) // buffered channel to avoid blocking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move these constants to the top of the file (defined in global space) and also list how these numbers were selected (state arbitrarily if so)
@@ -137,6 +137,63 @@ func (o *OrderbookUseCaseImpl) ProcessPool(ctx context.Context, pool sqsdomain.P | |||
return nil | |||
} | |||
|
|||
// GetActiveOrdersStream implements mvc.OrderBookUsecase. | |||
func (o *OrderbookUseCaseImpl) GetActiveOrdersStream(ctx context.Context, address string) <-chan orderbookdomain.ActiveOrder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have covered this method with tests here
2e5aabd
to
569abfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Outside diff range and nitpick comments (5)
domain/mocks/orderbook_usecase_mock.go (2)
42-47
: LGTM: NewGetActiveOrdersStream
method implementationThe implementation of
GetActiveOrdersStream
is correct and consistent with the mock's design pattern. It properly supports the SSE functionality described in the PR objectives.For consistency with other methods, consider adding a comment describing the method's purpose:
// GetActiveOrdersStream mocks the method for streaming active orders func (m *OrderbookUsecaseMock) GetActiveOrdersStream(ctx context.Context, address string) <-chan orderbookdomain.ActiveOrder { // ... (existing implementation) }
Line range hint
1-47
: Consider updating tests to leverage the new mock capabilityWhile this mock file itself doesn't require direct testing, it enables more comprehensive testing of components that depend on the
OrderBookUsecase
interface. To work towards the 50% coverage goal mentioned in the PR comments, consider the following:
- Update existing tests that use this mock to include scenarios that test the new SSE functionality.
- Add new tests for components that will use the
GetActiveOrdersStream
method.- Ensure that both the streaming (SSE) and non-streaming behaviors are tested in dependent components.
Would you like assistance in identifying files that might need updated tests or in generating example test cases for the new SSE functionality?
domain/orderbook/order.go (1)
97-101
: Improve documentation and consider adding a constructor forActiveOrder
- The comment on
LimitOrders
is inconsistent with its type. Consider updating it to accurately describe the field.- Add comments for
IsBestEffort
andError
fields to improve clarity.- Consider using a pointer for the
Error
field (*error
) to distinguish between a nil error and an empty error value.- Consider adding a constructor function for
ActiveOrder
to ensure proper initialization.Here's a suggested improvement:
type ActiveOrder struct { LimitOrders []LimitOrder // Slice of limit orders associated with this active order IsBestEffort bool // Indicates whether the order execution is best-effort Error *error // Stores any error that occurred during order processing } // NewActiveOrder creates a new ActiveOrder instance func NewActiveOrder(orders []LimitOrder, isBestEffort bool) *ActiveOrder { return &ActiveOrder{ LimitOrders: orders, IsBestEffort: isBestEffort, } }passthrough/delivery/http/passthrough_handler.go (1)
75-129
: Add unit tests forGetActiveOrdersStream
to improve test coverageTo ensure the reliability of the new Server-Sent Events functionality and to meet the minimum coverage threshold, consider adding unit tests for
GetActiveOrdersStream
. This will help catch potential issues early and maintain the overall quality of the codebase.Would you like assistance in writing these unit tests or opening a GitHub issue to track this task?
orderbook/usecase/orderbook_usecase.go (1)
172-176
: Handle possible blocking when channel buffer is fullWhen sending to the channel
c
, if the buffer is full and the receiver is slow or not consuming the messages, the goroutine will block. This can lead to goroutines piling up and potential deadlocks. Consider adding a timeout or handling this scenario to prevent blocking.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
orderbook/usecase/export_test.go
is excluded by!**/*_test.go
orderbook/usecase/orderbook_usecase_test.go
is excluded by!**/*_test.go
passthrough/delivery/http/passthrough_handler_test.go
is excluded by!**/*_test.go
Files selected for processing (7)
- app/sidecar_query_server.go (1 hunks)
- delivery/http/event.go (1 hunks)
- domain/mocks/orderbook_usecase_mock.go (2 hunks)
- domain/mvc/orderbook.go (1 hunks)
- domain/orderbook/order.go (1 hunks)
- orderbook/usecase/orderbook_usecase.go (1 hunks)
- passthrough/delivery/http/passthrough_handler.go (3 hunks)
Additional comments not posted (8)
domain/mvc/orderbook.go (1)
20-21
: Add tests for the new GetActiveOrdersStream methodTo address the comment about test coverage and meet the goal of 50% coverage, it's important to add tests for this new method. Testing asynchronous methods that return channels can be challenging, but it's crucial for ensuring the reliability of the SSE implementation.
Consider adding the following tests:
- Test that the method returns a channel.
- Test that the channel receives the expected
ActiveOrder
objects.- Test the behavior when the context is canceled.
- Test with various input addresses, including edge cases.
Here's a shell script to check for existing tests related to this method:
If these tests don't exist, please add them to ensure proper coverage of the new functionality.
domain/mocks/orderbook_usecase_mock.go (1)
18-18
: LGTM: New field for mockingGetActiveOrdersStream
The addition of
GetActiveOrdersStreamFunc
is consistent with the existing mock structure and allows for flexible mocking of the newGetActiveOrdersStream
method.domain/orderbook/order.go (1)
97-101
: Verify the relationship betweenActiveOrder
and SSE implementationThe new
ActiveOrder
type seems to be related to the Server-Sent Events (SSE) implementation mentioned in the PR objectives. To ensure proper implementation:
- Confirm that this type is indeed used for SSE responses in the Active Orders Query endpoint.
- Verify if the
IsBestEffort
field is related to the real-time nature of SSE updates.- Ensure that the
Error
field is properly utilized to communicate issues during the SSE stream.It would be helpful to see the code that uses this
ActiveOrder
type in the context of the SSE implementation to fully understand its role and ensure it meets the PR objectives.To help verify the usage of
ActiveOrder
in the SSE implementation, you can run the following script:This script will help identify where and how the
ActiveOrder
type is being used, particularly in relation to the SSE implementation.app/sidecar_query_server.go (2)
206-206
: Consider adding tests for the new SSE functionalityWhile this change is minimal, it's part of implementing Server-Sent Events (SSE) for the Active Orders Query endpoint. To address the concern raised in the PR comments about test coverage:
- Consider adding unit tests for the
NewPassthroughHandler
function to ensure it correctly handles the newlogger
parameter.- Implement integration tests that verify the SSE functionality of the Active Orders Query endpoint, including both SSE and non-SSE scenarios.
These additions would help achieve the suggested 50% coverage threshold and ensure the reliability of the new feature.
To check the current test coverage for the passthrough package, run the following command:
#!/bin/bash # Description: Check test coverage for the passthrough package go test -coverprofile=coverage.out ./passthrough/... go tool cover -func=coverage.outThis will help identify areas where additional test coverage might be needed.
206-206
: LGTM! Consider verifying theNewPassthroughHandler
implementation.The addition of the
logger
parameter to theNewPassthroughHandler
function call is a good improvement for observability. This change aligns well with the implementation of Server-Sent Events (SSE) for the Active Orders Query endpoint, as mentioned in the PR objectives.To ensure the logger is properly utilized, please run the following script to check the
NewPassthroughHandler
function implementation:This will help confirm that the
logger
parameter is being used effectively in theNewPassthroughHandler
function.Verification successful
Verification Successful: Logger Usage Confirmed
The
logger
parameter added to theNewPassthroughHandler
function is properly utilized within the handler's methods for error logging. This enhancement improves observability and aligns with the implementation of Server-Sent Events (SSE) for the Active Orders Query endpoint.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the NewPassthroughHandler function is using the logger parameter # Search for the NewPassthroughHandler function definition and its usage of the logger rg --type go -A 10 'func NewPassthroughHandler.*logger' passthrough/delivery/http/Length of output: 1451
Script:
#!/bin/bash # Description: Verify that the logger is being used within the handler's methods # Search for usages of the logger within passthrough_handler.go rg 'handler\.Logger\.' passthrough/delivery/http/passthrough_handler.goLength of output: 73
Script:
#!/bin/bash # Description: Verify all usages of the logger within passthrough_handler.go # Search for any usages of Logger within passthrough_handler.go rg 'Logger\.' passthrough/delivery/http/passthrough_handler.goLength of output: 209
delivery/http/event.go (1)
95-95
: Verify thatw.Flush()
is supported byecho.Response
Ensure that the
Flush()
method is available and properly supported by theecho.Response
object to prevent potential runtime errors.Run the following script to confirm that
echo.Response
supportsFlush()
:passthrough/delivery/http/passthrough_handler.go (1)
42-42
: Remove redundant route registration for '/active-orders'At line 42, the route
/active-orders
is registered twice. The first registration withhandler.GetActiveOrders
is immediately overridden by the second registration starting at line 43. Registering the same route twice can lead to unexpected behavior, as the first registration will be ignored.Apply this diff to remove the redundant route registration:
- e.GET(formatPassthroughResource("/active-orders"), handler.GetActiveOrders)
Likely invalid or redundant comment.
orderbook/usecase/orderbook_usecase.go (1)
140-148
: Reminder: Move constants to the top of the fileAs previously mentioned, please move the constants
fetchActiveOrdersDuration
andgetActiveOrdersStreamChanLen
to the top of the file for consistency with other constant declarations.
Implements SSE for Active Orders Query endpoint.
569abfd
to
cc3d892
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
passthrough/delivery/http/passthrough_handler.go (1)
Line range hint
1-180
: Overall assessment: Well-implemented SSE support with minor suggestions for improvement.This PR successfully implements Server-Sent Events (SSE) for the Active Orders Query endpoint, meeting the stated objectives. The changes are well-structured, maintain backward compatibility, and follow good coding practices. Here's a summary of the key points:
- The SSE implementation is correctly triggered by the
sse
query parameter.- Error handling and logging have been implemented consistently throughout the changes.
- The new helper function
parseAndValidateGetActiveOrdersRequest
improves code reuse and maintainability.- The
GetActiveOrdersStream
method follows SSE best practices, with minor suggestions for improvement regarding response flushing.Regarding the comment about test coverage, I recommend adding unit tests for the new
GetActiveOrdersStream
method and theparseAndValidateGetActiveOrdersRequest
helper function to improve the overall test coverage.To further improve the implementation, consider the following:
- Add a configuration option for the SSE keep-alive interval to prevent proxy timeouts.
- Implement a mechanism to handle client disconnects gracefully, ensuring that server resources are properly cleaned up.
- Consider adding metrics to track SSE connections and events for monitoring purposes.
These enhancements would make the SSE implementation more robust and easier to maintain in a production environment.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
orderbook/usecase/export_test.go
is excluded by!**/*_test.go
orderbook/usecase/orderbook_usecase_test.go
is excluded by!**/*_test.go
passthrough/delivery/http/passthrough_handler_test.go
is excluded by!**/*_test.go
Files selected for processing (7)
- app/sidecar_query_server.go (1 hunks)
- delivery/http/event.go (1 hunks)
- domain/mocks/orderbook_usecase_mock.go (2 hunks)
- domain/mvc/orderbook.go (1 hunks)
- domain/orderbook/order.go (2 hunks)
- orderbook/usecase/orderbook_usecase.go (2 hunks)
- passthrough/delivery/http/passthrough_handler.go (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/sidecar_query_server.go
- delivery/http/event.go
- domain/mocks/orderbook_usecase_mock.go
- domain/mvc/orderbook.go
- domain/orderbook/order.go
Additional comments not posted (10)
passthrough/delivery/http/passthrough_handler.go (6)
10-10
: LGTM: New imports for logging.The addition of the
log
andzap
packages is appropriate for implementing logging functionality. Usingzap
is a good choice as it's a high-performance, structured logging library.Also applies to: 16-16
23-23
: LGTM: Logger field added to PassthroughHandler struct.The addition of the
Logger
field to thePassthroughHandler
struct is a good practice. It allows for dependency injection of the logger, enhancing flexibility and testability of the code.
33-33
: LGTM: NewPassthroughHandler updated to include logger.The
NewPassthroughHandler
function has been correctly updated to accept alogger
parameter and assign it to thePassthroughHandler
struct. This change is consistent with the addition of theLogger
field and follows good dependency injection practices.Also applies to: 37-37
42-47
: LGTM: Routing updated to support SSE.The routing logic has been updated to support Server-Sent Events (SSE) for the
/active-orders
endpoint. The implementation checks for thesse
query parameter and routes to the appropriate handler. This approach maintains backward compatibility while adding the new SSE functionality, which is a good practice.
75-89
: LGTM: New helper function for request parsing and validation.The
parseAndValidateGetActiveOrdersRequest
helper function is a good addition. It encapsulates the request unmarshalling and validation logic, promoting code reuse and maintainability. This follows the DRY principle and improves the overall structure of the code.
167-168
: LGTM: GetActiveOrders updated to use new helper function.The
GetActiveOrders
method has been updated to use the newparseAndValidateGetActiveOrdersRequest
helper function. This change improves code consistency and reduces duplication, aligning with the DRY principle.orderbook/usecase/orderbook_usecase.go (4)
140-148
: LGTM! Constants are well-defined and explained.The addition of
fetchActiveOrdersDuration
andgetActiveOrdersStreamChanLen
with their explanatory comments addresses the previous review comment about explaining how these numbers were selected. The values seem reasonable for their intended purposes.
150-209
: LGTM! SSE implementation looks good.The
GetActiveOrdersStream
method successfully implements server-sent events for active orders, aligning with the PR objectives. The use of a buffered channel and a ticker for periodic fetching are appropriate approaches.
Line range hint
218-247
: LGTM! Improvements toGetActiveOrders
method.The modifications to the
GetActiveOrders
method align well with the newOrderbookResult
type. The changes provide more structured information and improve error handling and logging while maintaining the existing functionality.
Line range hint
1-577
: Overall, the changes look good with room for improvement.The implementation of Server-Sent Events (SSE) for the Active Orders Query endpoint is well done and aligns with the PR objectives. The code is generally well-structured and maintains existing functionality while adding new features.
Regarding the question about test coverage:
To address p0mvn's question about test coverage, let's check the current test coverage for this file:
This will help determine if additional tests are needed to meet the 50% coverage threshold. Based on the results, we can decide if more tests should be added, particularly for the new
GetActiveOrdersStream
method.
2674eee
to
81d5aa6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
validator/validator.go (1)
3-6
: LGTM! Consider enhancing the comment for better documentation.The
Validator
interface is well-defined and follows Go conventions. It adheres to the single responsibility principle, which is excellent.Consider enhancing the comment to provide more context:
-// Validator is any type capable to validate and having Validate method attached. +// Validator is an interface for types that can perform self-validation. +// Any type implementing this interface must provide a Validate method +// that returns an error if the validation fails, or nil if it succeeds.delivery/http/trace.go (2)
10-15
: LGTM:Span
function is well-implemented.The function correctly retrieves the current span from the context. It's concise and aligns with the PR objective of implementing SSE for the Active Orders Query endpoint.
Consider enhancing the function documentation slightly:
-// Span returns the current span from the context. +// Span returns the current span and its context from the given echo.Context.This provides a bit more clarity on what the function returns.
17-23
: LGTM:RecordSpanError
function is well-implemented with a minor suggestion.The function correctly records errors on the span and includes a helpful comment about span lifecycle management.
Consider adding error severity or categorization:
func RecordSpanError(ctx context.Context, span trace.Span, err error) { if err != nil { - span.RecordError(err) + span.RecordError(err, trace.WithAttributes( + attribute.String("error.type", fmt.Sprintf("%T", err)), + )) } // Note: we do not end the span here as it is ended in the middleware. }This addition would provide more context about the error in the trace, which could be helpful for debugging and monitoring.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- delivery/http/http.go (2 hunks)
- delivery/http/trace.go (1 hunks)
- passthrough/delivery/http/passthrough_handler.go (4 hunks)
- validator/validator.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- passthrough/delivery/http/passthrough_handler.go
Additional comments not posted (7)
validator/validator.go (2)
1-11
: Address PR objectives and test coverageWhile this
validator
package provides a useful abstraction for validation, it doesn't directly address the PR's main objective of implementing Server-Sent Events (SSE) for the Active Orders Query endpoint. Could you clarify how this package fits into the larger implementation?Additionally, as mentioned in the PR comments, there's a goal to achieve 50% test coverage. Consider adding unit tests for this package to contribute to that goal. Here's a simple example of what those tests might look like:
package validator import ( "errors" "testing" ) type testValidator struct { shouldPass bool } func (tv testValidator) Validate() error { if tv.shouldPass { return nil } return errors.New("validation failed") } func TestValidate(t *testing.T) { tests := []struct { name string v Validator wantErr bool }{ {"passing validation", testValidator{true}, false}, {"failing validation", testValidator{false}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if err := Validate(tt.v); (err != nil) != tt.wantErr { t.Errorf("Validate() error = %v, wantErr %v", err, tt.wantErr) } }) } }To check the current test coverage for this package:
#!/bin/bash # Description: Check test coverage for the validator package # Run tests with coverage go test -coverprofile=coverage.out ./validator # Display coverage statistics go tool cover -func=coverage.out # Clean up rm coverage.outThis will help assess the current test coverage and identify areas that need improvement to reach the 50% goal.
8-11
: LGTM! Consider enhancing documentation and evaluating function necessity.The
Validate
function correctly uses theValidator
interface. However, there are a couple of points to consider:
- Enhance the comment for better documentation:
-// Validate validates type v. +// Validate calls the Validate method on the given Validator. +// It returns an error if the validation fails, or nil if it succeeds.
- Consider if this function is necessary. It doesn't add much functionality beyond calling
v.Validate()
directly. If it's not used in multiple places or doesn't provide additional context or error handling, you might want to remove it and use the interface method directly where needed.To help decide if this function is necessary, let's check its usage:
This will help determine if the
Validate
function is being used elsewhere in the codebase, which would justify its existence.Verification successful
Approve Removal of
Validate
FunctionThe
Validate
function is only used indelivery/http/http.go
. Consider removing it and replacing its usage with direct calls to theValidator
interface to simplify the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of Validate function across the codebase # Search for import statements of the validator package echo "Imports of the validator package:" rg --type go "import.*\".*validator.*\"" # Search for usage of the Validate function echo "\nUsage of Validate function:" rg --type go "validator\.Validate\("Length of output: 559
delivery/http/trace.go (2)
1-8
: LGTM: Package declaration and imports are appropriate.The package name and imports are well-chosen and relevant to the functionality provided in this file.
1-23
: Great addition of tracing utilities, consider adding tests.This file introduces well-implemented OpenTelemetry tracing utilities that align with the PR objective of implementing SSE for the Active Orders Query endpoint. The code is clean, well-structured, and follows Go best practices.
To address the comment about testing coverage, consider adding unit tests for these functions. Here's a script to check the current test coverage:
This will help ensure we meet the 50% coverage threshold mentioned in the PR comments.
delivery/http/http.go (3)
5-5
: LGTM: Import statement added correctly.The import for the
validator
package is correctly added and is necessary for the new functionality in theParseRequest
function.
17-30
: Consider adding tests and provide more context on SSE implementation.While the
ParseRequest
function is a valuable addition, it doesn't directly address the Server-Sent Events (SSE) functionality mentioned in the PR objectives. To align with the project's goals:
Consider adding unit tests for the
ParseRequest
function to improve test coverage. This would help address the concern raised about reaching the 50% coverage threshold.Could you provide more information on how this change relates to the SSE implementation for the Active Orders Query endpoint? Are there additional files or changes that will be part of this PR to fully implement the SSE functionality?
To help understand the current test coverage and identify areas for improvement, we can run the following command:
#!/bin/bash # Description: Check current test coverage for the delivery/http package # Run tests with coverage go test ./delivery/http -coverprofile=coverage.out # Display coverage statistics go tool cover -func=coverage.out # Clean up rm coverage.outThis will help us identify which functions might need additional test coverage.
18-30
: LGTM: Well-implementedParseRequest
function, but clarification needed.The
ParseRequest
function is well-implemented, encapsulating both unmarshalling and validation logic. It correctly uses type assertion to check if the request implements theValidator
interface before performing validation. This approach provides flexibility and adheres to the single responsibility principle.However, I don't see a direct connection between this function and the Server-Sent Events (SSE) functionality mentioned in the PR objectives. Could you please clarify how this function contributes to the SSE implementation for the Active Orders Query endpoint?
To verify the usage of this new function and its potential relation to SSE, let's search for its usage in the codebase:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
domain/mvc/orderbook.go
Outdated
@@ -16,4 +16,9 @@ type OrderBookUsecase interface { | |||
|
|||
// GetOrder returns all active orderbook orders for a given address. | |||
GetActiveOrders(ctx context.Context, address string) ([]orderbookdomain.LimitOrder, bool, error) | |||
|
|||
// GetActiveOrdersStream returns a channel for streaming limit orderbook orders for a given address. | |||
// The caller should range over the channel, but note that channel is never closed since there may by multiple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is then responsible for closing it to prevent leakage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since channels are not like files it's fine to keep those open. Closing is necessary when we want to inform client that there will be no values send, which is simple to achieve with one sender, but difficult with mulitple senders, because sending to close channel would cause a panic. In our scenario sender will have always some values to sent, thus I think adding synchronization to close a channel would only introduce unecessary complexity without clear benefit.
c := make(chan orderbookdomain.OrderbookResult, getActiveOrdersStreamChanLen) | ||
|
||
// Function to fetch orders | ||
fetchOrders := func(ctx context.Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we could reuse this in GetActiveOrders
to reduce code duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think atm code is quite opimised it for duplication: we're reusing common data structures; I think further optmisations would add not significant improvements vs time spent finding right abstractions due smalll differences between fetchOrders
and GetActiveOrders
. For example in fetchOrders
we skip empty results, have additional logging specifc for this method.
if c.QueryParam("sse") != "" { | ||
return handler.GetActiveOrdersStream(c) // Server-Sent Events (SSE) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any consideration that we should make wrt load balancing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a reference: discussed via slack.
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
orderbook/usecase/orderbook_usecase.go (2)
140-148
: Consider adding comments to explain the rationale behind constant values.The new constants
fetchActiveOrdersDuration
andgetActiveOrdersStreamChanLen
are well-named, but it would be helpful to add comments explaining why these specific values were chosen. This would provide context for future developers and make it easier to adjust these values if needed.
Line range hint
218-247
: Approve changes with a suggestion for error handling.The changes to the
GetActiveOrders
method improve type safety and error logging. The use oforderbookdomain.OrderbookResult
enhances consistency with the new streaming method.Consider aggregating errors from all goroutines instead of just logging them. This would provide a more comprehensive error report to the caller.
You could modify the error handling like this:
var errs []error for i := 0; i < len(orderbooks); i++ { select { case result := <-results: if result.Error != nil { telemetry.ProcessingOrderbookActiveOrdersErrorCounter.Inc() o.logger.Error(telemetry.ProcessingOrderbookActiveOrdersErrorMetricName, zap.Any("pool_id", result.PoolID), zap.Any("err", result.Error)) + errs = append(errs, result.Error) } isBestEffort = isBestEffort || result.IsBestEffort finalResults = append(finalResults, result.LimitOrders...) case <-ctx.Done(): return nil, false, ctx.Err() } } +if len(errs) > 0 { + return finalResults, isBestEffort, fmt.Errorf("errors occurred while processing orderbooks: %v", errs) +} return finalResults, isBestEffort, nil
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- domain/mvc/orderbook.go (1 hunks)
- orderbook/usecase/orderbook_usecase.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- domain/mvc/orderbook.go
This PR implements SSE for Active Orders Query endpoint.
Implementation extends existing functionality by looking for
sse
URL param for existing endpoint, for example:/passthrough/active-orders?userOsmoAddress=osmo1jgz4xmaw9yk9pjxd4h8c2zs0r0vmgyn88s8t6l&sse=1
When there is nosse
passed, endpoint will return a list of orders as per its original functionlity.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These changes improve the overall user experience by providing real-time data and more robust logging.